Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Black text not visible #5291 #5409

Merged
merged 1 commit into from
Oct 25, 2014
Merged

Black text not visible #5291 #5409

merged 1 commit into from
Oct 25, 2014

Conversation

thejdeep
Copy link
Contributor

Waiting for test

@timvandermeij
Copy link
Contributor

@thejdeep Could you squash your commits into one commit? See https://github.com/mozilla/pdf.js/wiki/Squashing-Commits on how to do this easily.

@thejdeep
Copy link
Contributor Author

@yurydelendik any updates on including the test PDF ?

@yurydelendik
Copy link
Contributor

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/1885863c83a7241/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://107.22.172.223:8877/1885863c83a7241/output.txt

Total script time: 17.86 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://107.22.172.223:8877/1885863c83a7241/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator

With this PR the text (in #5291) is visible, but compared to the rendering in Adobe Reader it unfortunately doesn't really fix the issue:
difference

@yurydelendik
Copy link
Contributor

Also, tests are failing. Something looks wrong about this.current.fontSizeScale calculation.


var browserFontSize = size < MIN_FONT_SIZE ? MIN_FONT_SIZE :
size > MAX_FONT_SIZE ? MAX_FONT_SIZE : size;
this.current.fontSizeScale = size/browserFontSize;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add spaces around '/'

@Snuffleupagus
Copy link
Collaborator

@timvandermeij
Copy link
Contributor

@thejdeep Could you squash the commits again?

@thejdeep
Copy link
Contributor Author

@timvandermeij Squashing merge commits diverges my branch from origin/master . Help

@yurydelendik
Copy link
Contributor

Following either of first two sections at https://github.com/mozilla/pdf.js/wiki/Squashing-Commits shall fix any diverging. (https://github.com/mozilla/pdf.js/pull/5409.diff shall contain the current branch's diff, you can also reset your "master" git checkout -B master upstream/master and apply this diff) ("master" is okay, but for future reference, try to follow https://github.com/mozilla/pdf.js/wiki/Contributing)

@CodingFabian
Copy link
Contributor

the main problem is that you sent a pull request from master. never do that. always create a feature branch that avoids many issues.

@yurydelendik
Copy link
Contributor

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/200a658ae707aca/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/200a658ae707aca/output.txt

Total script time: 18.35 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

yurydelendik added a commit that referenced this pull request Oct 25, 2014
@yurydelendik yurydelendik merged commit 1bf72f3 into mozilla:master Oct 25, 2014
@yurydelendik
Copy link
Contributor

Thank you for the patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants